feat: moving cli config to --set for nodeport and storageclass #2083

bdw617 · 2023-10-18T19:05:44Z

Description

replace -nodeport and --storageclass: zarf init --nodeport <number> --storage-class <my-storage-class>
with: zarf init --set REGISTRY_NODEPORT=31998 --set REGISTRY_STORAGE_CLASS=my-storage-class

old variables still exist and work, but warnings are given to the user if they use them as deprecated.

NOTE: storageClass to be used for gitea and the registry will by default be the default storageClass of the cluster.

Related Issue

Fixes #1725

Type of change

Bug fix (non-breaking change which fixes an issue)
[x ] New feature (non-breaking change which adds functionality)
Other (security config, docs update, etc)

Checklist before merging

[x ] Test, docs, adr added or updated as needed
[x ] Contributor Guide Steps followed

netlify · 2023-10-18T19:05:49Z

✅ Deploy Preview for zarf-docs canceled.

Name	Link
🔨 Latest commit	`5e7e182`
🔍 Latest deploy log	https://app.netlify.com/sites/zarf-docs/deploys/656534e212096f000893ec84

…till be there, just not in the output

…nd removed from the zarf init --help

…e main init options

…CLASS=local-path

…e the cluster's default storage class. This should be backwards compatible with any deployment that is using the default storage class, without having to put it into zarf state

…appears to work better

…d needs a refactor later

…e config

lucasrod16 · 2023-11-30T19:21:45Z

src/cmd/initialize.go

To be more consistent with other deprecated fields/flags in the codebase, it would probably be good to rename the deprecated fields to DeprecatedStorageClass in the ZarfInitOptions struct, and DeprecatedNodePort in the RegistryInfo struct. This way we wouldn't need to add the nodePortArg and storageClassArg variables or remove any struct fields at this time.

lucasrod16 · 2023-11-30T19:28:25Z

src/cmd/initialize.go

+
+	if internalRegistry {
+		if configuredNodePort > 32767 || configuredNodePort < 30000 {
+			configuredNodePort = config.ZarfInClusterContainerRegistryNodePort


Would probably be good to notify the user that they provided an invalid nodeport number and that we're going to use the default Zarf nodeport of 31999 instead. Might save users some confusion on why their specified port number wasn't used by Zarf.

lucasrod16 · 2023-11-30T20:00:27Z

src/cmd/initialize.go

+		pkgConfig.InitOpts.RegistryInfo.NodePort = configuredNodePort
+	} else {
+		// do not set the nodeport if this is an external registry
+		pkgConfig.PkgOpts.SetVariables[configVar] = ""


We should be able to remove this else block. If the user specifies an external registry, we shouldn't have to set the REGISTRY_NODEPORT var to an empty string

lucasrod16 · 2023-11-30T20:10:15Z

src/cmd/initialize_test.go

+	storageClassArg = ""
+	setRegistryStorageClass()
+	if storageClassArg != "test-storage-class" {
+		t.Errorf("Expected storage class to be set to 'test-storage-class', but got '%s'", storageClassArg)


We should use require.Equal() to be more consistent with the rest of our test suite assertions. When require.Equal() fails it prints an "expected but got" message by default for you.

lucasrod16 · 2023-11-30T20:11:35Z

src/cmd/initialize_test.go

+	storageClassArg = "old-storage-class"
+	setRegistryStorageClass()
+	if pkgConfig.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"] != "old-storage-class" {
+		t.Errorf("Expected storage class to be set to old way value, but got '%s'", pkgConfig.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"])


We should use require.Equal() to be more consistent with the rest of our test suite assertions. When require.Equal() fails it prints an "expected but got" message by default for you.

lucasrod16 · 2023-11-30T20:11:49Z

src/cmd/initialize_test.go

+	storageClassArg = ""
+	setRegistryStorageClass()
+	if pkgConfig.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"] != "" {
+		t.Errorf("Expected storage class to be set to empty string, but got '%s'", pkgConfig.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"])


We should use require.Equal() to be more consistent with the rest of our test suite assertions. When require.Equal() fails it prints an "expected but got" message by default for you.

lucasrod16 · 2023-11-30T20:32:32Z

src/pkg/packager/deploy.go

+		}
+
+		if err != nil {
+			message.WarnErr(err, "Unable to get the default storage class from the cluster")


I think we have an opportunity to fail fast here. If REGISTRY_PVC_ENABLED is true (this is the default), the user did not specify a storage class to use, and there was no default storage class found in the cluster, we should fail fast and notify the user that a storage class is needed for the registry to deploy successfully.

RKE2 clusters come to mind since they don't provide a default storage class

Racer159 · 2023-11-29T22:40:16Z

packages/gitea/gitea-values.yaml

@@ -1,5 +1,5 @@
 persistence:
-  storageClass: "###ZARF_STORAGE_CLASS###"
+  storageClass: "###ZARF_VAR_REGISTRY_STORAGE_CLASS###"


Suggested change

storageClass: "###ZARF_VAR_REGISTRY_STORAGE_CLASS###"

storageClass: "###ZARF_VAR_GIT_SERVER_STORAGE_CLASS###"

Racer159 · 2023-11-29T22:40:43Z

packages/zarf-registry/zarf.yaml

+    default: "31999"
+    autoIndent: true
+
+  - name: REGISTRY_STORAGE_CLASS


There should be a corresponding variable for the GIT_SERVER

Racer159 · 2023-11-29T22:41:54Z

src/cmd/initialize.go

+
+// DEPRECATED_V1.0.0: --storage-class should be removed from the CLI in v1.0.0
+func setRegistryStorageClass() {
+	configVar := "REGISTRY_STORAGE_CLASS"


This would set both the registry and git server vars

Racer159 · 2023-11-29T22:45:23Z

src/config/lang/english.go

+// Zarf CLI deprecation warnings
+const (
+	// remove deprecation in v1.0.0
+	WarnStorageClassDeprecated = "--storage-class <value> is deprecated, please use --set REGISTRY_STORAGE_CLASS=<value>"


Suggested change

WarnStorageClassDeprecated = "--storage-class <value> is deprecated, please use --set REGISTRY_STORAGE_CLASS=<value>"

WarnStorageClassDeprecated = "--storage-class <value> is deprecated, please use --set REGISTRY_STORAGE_CLASS=<value> and --set GIT_SERVER_STORAGE_CLASS=<value>"

Racer159 · 2023-11-29T22:46:12Z

src/cmd/initialize.go

+
+	pkgConfig.InitOpts.RegistryInfo.InternalRegistry = internalRegistry
+
+	// TODO: test external registry and CLI.


Racer159 · 2023-11-29T22:50:04Z

src/config/lang/english.go

@@ -167,7 +174,7 @@ $ zarf init --artifact-push-password={PASSWORD} --artifact-push-username={USERNA
 	CmdInitFlagGitPullPass = "Password for the pull-only user to access the git server"

 	CmdInitFlagRegURL      = "External registry url address to use for this Zarf cluster"
-	CmdInitFlagRegNodePort = "Nodeport to access a registry internal to the k8s cluster. Between [30000-32767]"
+	CmdInitFlagRegNodePort = "[DEPRECATED] Nodeport to access a registry internal to the k8s cluster. Between [30000-32767]"


Suggested change

CmdInitFlagRegNodePort = "[DEPRECATED] Nodeport to access a registry internal to the k8s cluster. Between [30000-32767]"

CmdInitFlagRegNodePort = "[Deprecated] Nodeport to access a registry internal to the k8s cluster. Between [30000-32767] (use --set REGISTRY_NODEPORT=<value> instead)"

Racer159 · 2023-11-29T22:54:31Z

src/pkg/packager/deploy.go

+			p.cfg.PkgOpts.SetVariables = map[string]string{}
+		}
+		if c := p.cfg.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"]; c == "" {
+			p.cfg.PkgOpts.SetVariables["REGISTRY_STORAGE_CLASS"], err = p.cluster.GetDefaultStorageClass()


This shouldn't be needed if the storage class is empty

Tested this with k3d, kind, k3s, and minikube locally across zarf v0.31.3 and this build, new inits and upgrades... do you have a test case where this was needed?

Racer159 · 2023-12-03T02:33:42Z

src/internal/cluster/state.go

-		if newState.RegistryInfo.Address == fmt.Sprintf("%s:%d", helpers.IPV4Localhost, newState.RegistryInfo.NodePort) {
-			newState.RegistryInfo.InternalRegistry = true
-		} else {
-			newState.RegistryInfo.InternalRegistry = false
-		}


Removing this logic from here will break the library usages of zarf init - Nodeport should be pulled out of ZARF_VAR_REGISTRY_NODEPORT for now. (later this can be refactored to a more explicit mapping)

Racer159 · 2023-12-03T02:34:35Z

src/internal/packager/template/template.go

+		if values.config.SetVariableMap != nil {
+			if regInfo.NodePort > 0 && values.config.SetVariableMap["REGISTRY_NODEPORT"] != nil {
+				values.config.SetVariableMap["REGISTRY_NODEPORT"].Value = fmt.Sprintf("%d", regInfo.NodePort)
+			}
+			if values.config.State.StorageClass != "" && values.config.SetVariableMap["REGISTRY_STORAGE_CLASS"] != nil {
+				values.config.SetVariableMap["REGISTRY_STORAGE_CLASS"].Value = values.config.State.StorageClass
+			}
+		}


This doesn't need to be here - variables in a zarf.yaml will just pass through

Racer159 · 2023-12-03T03:13:22Z

src/types/runtime.go

+	// removed
+	//StorageClass string `json:"storageClass" jsonschema:"description=StorageClass of the k8s cluster Zarf is initializing"`


Commented code should be deleted - it will still be in git if we ever need it

lucasrod16 · 2024-05-08T19:26:39Z

Closing this PR because we will likely rethink the implementation

feat: moving cli config to --set for nodeport and storageclass

6a17d66

bdw617 requested review from jeff-mccoy, Racer159, Noxsios, Madeline-UX and lucasrod16 as code owners October 18, 2023 19:05

bdw617 and others added 3 commits October 18, 2023 16:05

updated docs to be consistent with deprecated CLI flags, which will s…

af54703

…till be there, just not in the output

updated fix docs test

b79bae9

Merge branch 'main' into issue-1725

74570e0

bdw617 marked this pull request as draft October 18, 2023 20:50

bdw617 and others added 19 commits October 19, 2023 09:36

removed a test for the existence of an init flag that is deprecated a…

3c0e9d5

…nd removed from the zarf init --help

we moved nodeport to --set REGISTRY_NODEPORT, and therefore not in th…

3005fb6

…e main init options

CI testing, not sure if necessary

d67f9e8

updated, using CI for testing

697554f

updated, ready for testing

4fbadc0

should work now, testing via CI

f49e4ad

fix test in CI

6576d6b

updated for ci tests, some will break

d5f7d35

Merge branch 'main' into issue-1725

0a7817f

fixed merge issue

4cd06a1

updated to fix an NPE in non-init

cf0d90a

updated

83175e1

moving to default storage class. makes us less distro complicated

7c2fde7

fix upgrade test, will need to release notes this with --set STORAGE_…

e355116

…CLASS=local-path

fixed typo

ae7eff1

updated so if REGISTRY_STORAGE_CLASS isn't set, it will attempt to us…

c1ed163

…e the cluster's default storage class. This should be backwards compatible with any deployment that is using the default storage class, without having to put it into zarf state

trying with just empty, fixing chart, need to update it.

b12026d

Merge branch 'main' into issue-1725

8042767

trying to always set the values for the storage class

5874c1c

bdw617 and others added 10 commits October 30, 2023 18:04

hack, will need a better way to template state into ###ZARF_VAR_...###

8240867

a bit of cleanup

8954308

Merge branch 'main' into issue-1725

431a292

immutable nodeport once created, need more testing

5c32128

updated

4fe956c

tried to force the nodeport to always be the same, didn't work, this …

7a043a6

…appears to work better

Merge branch 'main' into issue-1725

a5d9cf2

the address needs to change in this case

6698814

Merge branch 'main' into issue-1725

20f0821

Merge branch 'main' into issue-1725

878d29d

bdw617 added the needs-docs PR Label - Docs required to merge label Nov 15, 2023

bdw617 and others added 5 commits November 15, 2023 17:20

updated to preserve the zarf state as it was, it simplifies config an…

294ffbe

…d needs a refactor later

Merge branch 'main' into issue-1725

f36a71d

fixed NPE

4e977f1

must update this to the old way, since the release zarf won't have th…

5d21c11

…e config

added a documentation line for an External function

43acd57

bdw617 removed the needs-docs PR Label - Docs required to merge label Nov 17, 2023

Merge branch 'main' into issue-1725

db54848

bdw617 marked this pull request as ready for review November 27, 2023 14:55

bdw617 added 2 commits November 27, 2023 16:32

Merge branch 'main' into issue-1725

15eb1a8

Merge branch 'main' into issue-1725

5e7e182

lucasrod16 reviewed Nov 30, 2023

View reviewed changes

Racer159 requested changes Dec 3, 2023

View reviewed changes

lucasrod16 closed this May 8, 2024

lucasrod16 deleted the issue-1725 branch May 8, 2024 19:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: moving cli config to --set for nodeport and storageclass #2083

feat: moving cli config to --set for nodeport and storageclass #2083

bdw617 commented Oct 18, 2023 •

edited

Loading

netlify bot commented Oct 18, 2023 •

edited

Loading

lucasrod16 Nov 30, 2023

lucasrod16 Nov 30, 2023

lucasrod16 Nov 30, 2023

lucasrod16 Nov 30, 2023

lucasrod16 Nov 30, 2023

lucasrod16 Nov 30, 2023

lucasrod16 Nov 30, 2023

Racer159 Nov 29, 2023

Racer159 Nov 29, 2023

Racer159 Nov 29, 2023

Racer159 Nov 29, 2023

Racer159 Nov 29, 2023

Racer159 Nov 29, 2023

Racer159 Nov 29, 2023

Racer159 Dec 3, 2023

Racer159 Dec 3, 2023

Racer159 Dec 3, 2023

Racer159 Dec 3, 2023

lucasrod16 commented May 8, 2024

	storageClass: "###ZARF_VAR_REGISTRY_STORAGE_CLASS###"
	storageClass: "###ZARF_VAR_GIT_SERVER_STORAGE_CLASS###"

	WarnStorageClassDeprecated = "--storage-class <value> is deprecated, please use --set REGISTRY_STORAGE_CLASS=<value>"
	WarnStorageClassDeprecated = "--storage-class <value> is deprecated, please use --set REGISTRY_STORAGE_CLASS=<value> and --set GIT_SERVER_STORAGE_CLASS=<value>"


		pkgConfig.InitOpts.RegistryInfo.InternalRegistry = internalRegistry

		// TODO: test external registry and CLI.

	CmdInitFlagRegNodePort = "[DEPRECATED] Nodeport to access a registry internal to the k8s cluster. Between [30000-32767]"
	CmdInitFlagRegNodePort = "[Deprecated] Nodeport to access a registry internal to the k8s cluster. Between [30000-32767] (use --set REGISTRY_NODEPORT=<value> instead)"

		// removed
		//StorageClass string `json:"storageClass" jsonschema:"description=StorageClass of the k8s cluster Zarf is initializing"`

feat: moving cli config to --set for nodeport and storageclass #2083

feat: moving cli config to --set for nodeport and storageclass #2083

Conversation

bdw617 commented Oct 18, 2023 • edited Loading

Description

Related Issue

Type of change

Checklist before merging

netlify bot commented Oct 18, 2023 • edited Loading

✅ Deploy Preview for zarf-docs canceled.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucasrod16 commented May 8, 2024

bdw617 commented Oct 18, 2023 •

edited

Loading

netlify bot commented Oct 18, 2023 •

edited

Loading